Bump version to 6.0.0 and rename SQL scripts#401
Conversation
📝 WalkthroughWalkthroughThis PR finalizes the Spock 6.0.0 release by updating the version constant from ChangesRelease 6.0.0 and Schema Upgrade Validation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
868f54b to
1cf3438
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/tap/t/018_upgrade_schema_match.pl (1)
150-165: Minor: Temporary file cleanup on early exit.If
psql_querydies between creating$tmpfileand theunlink, the temp file is left behind. Consider using a block-scoped cleanup or wrapping in eval.♻️ Suggested improvement
sub psql_query { my ($port, $sql) = `@_`; my $tmpfile = "/tmp/spock_018_$$.sql"; + my $out; open my $fh, '>', $tmpfile or die "Cannot write $tmpfile: $!"; print $fh $sql; close $fh; - open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', - '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile - or die "Cannot run psql: $!"; - my $out = join '', <$fh>; - close $fh; - unlink $tmpfile; + eval { + open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', + '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile + or die "Cannot run psql: $!"; + $out = join '', <$fh>; + close $fh; + }; + my $err = $@; + unlink $tmpfile; + die $err if $err; chomp $out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 150 - 165, psql_query currently writes a temp file and may leave it behind if the function dies before the final unlink; modify psql_query to guarantee cleanup by using a safe temporary-file helper (e.g., File::Temp->new or a block-scoped temp file) or wrap the write/psql invocation in an eval/finally-like construct (local $@; eval { ... }; my $err = $@; unlink $tmpfile if -e $tmpfile; die $err if $err) so that the temp file is always removed; reference the psql_query function and the $tmpfile variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/tap/t/018_upgrade_schema_match.pl`:
- Around line 150-165: psql_query currently writes a temp file and may leave it
behind if the function dies before the final unlink; modify psql_query to
guarantee cleanup by using a safe temporary-file helper (e.g., File::Temp->new
or a block-scoped temp file) or wrap the write/psql invocation in an
eval/finally-like construct (local $@; eval { ... }; my $err = $@; unlink
$tmpfile if -e $tmpfile; die $err if $err) so that the temp file is always
removed; reference the psql_query function and the $tmpfile variable when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87b0828a-fe94-4595-8692-810148ddddff
⛔ Files ignored due to path filters (2)
tests/regress/expected/init_1.outis excluded by!**/*.outtests/regress/expected/init_fail.outis excluded by!**/*.out
📒 Files selected for processing (6)
include/spock.hsql/spock--5.0.6--5.1.0.sqlsql/spock--5.1.0.sqltests/regress/sql/init_fail.sqltests/tap/scheduletests/tap/t/018_upgrade_schema_match.pl
|
We will also need to pull in 5.0.7 |
1cf3438 to
e49aa36
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sql/spock--5.0.6--5.0.7.sql`:
- Line 37: Replace the invalid PL/pgSQL assignment-from-query "target_id :=
node_id FROM spock.node_info();" with a proper SELECT ... INTO form; e.g. use
"SELECT node_id INTO target_id FROM spock.node_info();" (or "SELECT INTO STRICT"
if you expect exactly one row) for both occurrences referencing target_id,
node_id and spock.node_info() (the instances around lines 37 and 109).
In `@tests/tap/t/018_upgrade_schema_match.pl`:
- Around line 276-280: The stop_pg helper ignores failures from pg_ctl which can
leave the old postmaster running; modify stop_pg (the function `stop_pg`) to
check the result/exit status of `run_logged("$PG_BIN/pg_ctl", 'stop', ...)` and
fail the test (or die) if pg_ctl returns non-zero, and optionally wait/retry
until the postmaster is confirmed stopped (e.g. poll for PID file removal or use
`pg_ctl status`) before returning so subsequent phase 6 upgrade assertions won't
run against a still-running old postmaster.
- Around line 156-161: The current code reads psql output from a piped
filehandle ($fh) but never checks the exit status of the spawned psql process,
which can return non-zero and produce empty output; after close $fh add a check
of the child's exit status (e.g., inspect $? and die with a clear error
including $tmpfile, the command and exit code) so that when the psql invocation
(the open using "$PG_BIN/psql" with $tmpfile) fails the test fails fast instead
of using empty output for schema matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99ba0d47-b3ac-43ef-b218-8c30016e8fcb
⛔ Files ignored due to path filters (2)
tests/regress/expected/init_1.outis excluded by!**/*.outtests/regress/expected/init_fail.outis excluded by!**/*.out
📒 Files selected for processing (7)
include/spock.hsql/spock--5.0.6--5.0.7.sqlsql/spock--5.0.7--6.0.0.sqlsql/spock--6.0.0.sqltests/regress/sql/init_fail.sqltests/tap/scheduletests/tap/t/018_upgrade_schema_match.pl
✅ Files skipped from review due to trivial changes (2)
- include/spock.h
- tests/tap/schedule
| open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', | ||
| '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile | ||
| or die "Cannot run psql: $!"; | ||
| my $out = join '', <$fh>; | ||
| close $fh; | ||
| unlink $tmpfile; |
There was a problem hiding this comment.
Fail fast when psql_query execution fails.
Line 156–Line 161 do not validate the piped psql exit status. A failed query can collapse to empty output and produce false-positive schema matches.
💡 Proposed fix
my $out = join '', <$fh>;
- close $fh;
+ my $ok = close $fh;
+ my $rc = $? >> 8;
unlink $tmpfile;
+ die "psql query failed (rc=$rc) for $tmpfile" if !$ok || $rc != 0;
chomp $out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 156 - 161, The current
code reads psql output from a piped filehandle ($fh) but never checks the exit
status of the spawned psql process, which can return non-zero and produce empty
output; after close $fh add a check of the child's exit status (e.g., inspect $?
and die with a clear error including $tmpfile, the command and exit code) so
that when the psql invocation (the open using "$PG_BIN/psql" with $tmpfile)
fails the test fails fast instead of using empty output for schema matching.
- Rename sql/spock--6.0.0-devel.sql → spock--6.0.0.sql - Rename sql/spock--5.0.6--6.0.0-devel.sql → spock--5.0.8--6.0.0.sql - Add sql/spock--5.0.6--5.0.7.sql step upgrade script - Add sql/spock--5.0.7--5.0.8.sql step upgrade script (no schema changes in 5.0.8) - Bump version in include/spock.h - Update regression test fixtures for 6.0.0 version strings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ma parity TAP test that installs spock 5.0.8, upgrades to 6.0.0, and compares the resulting schema against a fresh 6.0.0 install — covering tables, columns, functions, views, sequences, view definitions, and function bodies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e49aa36 to
09929f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sql/spock--5.0.8--6.0.0.sql (1)
289-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
%Iwith%sfor theregclassparameterrel.Since
relis declared asregclass(line 220), it renders as a schema-qualified relation name. Using%Itreats the entire text as a single identifier, producing"public.my_table"instead ofpublic.my_table, which breaks both theSECURITY LABEL ON COLUMNandALTER TABLEstatements. Use%sinstead, as regclass output is already safely-escaped.Fix locations
- Line 290:
SECURITY LABEL FOR spock ON COLUMN %I.%I→ change first%Ito%s- Line 293:
SECURITY LABEL FOR spock ON COLUMN %I.%I→ change first%Ito%s- Line 316:
ALTER TABLE %I REPLICA IDENTITY FULL→ change%Ito%s- Line 323:
ALTER TABLE %I REPLICA IDENTITY DEFAULT→ change%Ito%s🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sql/spock--5.0.8--6.0.0.sql` around lines 289 - 323, The SQL uses the regclass variable rel with format() and %I, which quotes the whole schema-qualified name; update the format calls that build sqlstring and ALTER TABLE to use %s for rel instead of %I: change the first %I to %s in both SECURITY LABEL FOR spock ON COLUMN format() calls (the ones that build sqlstring) and change %I to %s in the two ALTER TABLE format() calls so the regclass value renders as an unquoted schema-qualified name; keep %I for att_name and other true identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sql/spock--5.0.8--6.0.0.sql`:
- Around line 1-4: The script header and user message still target 5.0.8->6.0.0;
change both to reflect the correct upgrade source 5.0.7->6.0.0 by renaming the
file comment string "/* spock--5.0.8--6.0.0.sql */" to "/*
spock--5.0.7--6.0.0.sql */" (and if the actual filename is the old form, rename
the file accordingly) and update the psql echo text that contains "ALTER
EXTENSION spock UPDATE TO '6.0.0'" to clearly reference the 5.0.7 source (e.g.,
mirror the filename change in the message so the script and its user-facing
guidance consistently indicate 5.0.7 -> 6.0.0).
In `@tests/tap/t/018_upgrade_schema_match.pl`:
- Around line 286-330: The test's catalog queries miss index definitions and
sequence properties; update the diff queries by adding a new Q_INDEXES (e.g.,
query pg_catalog.pg_indexes or join pg_class/pg_index/pg_attribute) that returns
a deterministic string per index including schemaname.table, indexname, indexdef
(or columns/order, uniqueness, predicate, expression) ordered by index name, and
extend Q_SEQUENCES to include sequence attributes (data_type/udt_name,
start_value, minimum_value, maximum_value, increment_by, is_cycled,
last_value/owned_by if relevant) so the test will detect changes like
spock.resolutions(log_time) index removal or differences in
spock.sub_id_generator AS integer/CYCLE/start; apply the same sequence expansion
where the other query copy exists (lines noted in the comment).
---
Outside diff comments:
In `@sql/spock--5.0.8--6.0.0.sql`:
- Around line 289-323: The SQL uses the regclass variable rel with format() and
%I, which quotes the whole schema-qualified name; update the format calls that
build sqlstring and ALTER TABLE to use %s for rel instead of %I: change the
first %I to %s in both SECURITY LABEL FOR spock ON COLUMN format() calls (the
ones that build sqlstring) and change %I to %s in the two ALTER TABLE format()
calls so the regclass value renders as an unquoted schema-qualified name; keep
%I for att_name and other true identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 823492b3-11ae-4fbc-842a-10d682c2c343
⛔ Files ignored due to path filters (2)
tests/regress/expected/init_1.outis excluded by!**/*.outtests/regress/expected/init_fail.outis excluded by!**/*.out
📒 Files selected for processing (8)
include/spock.hsql/spock--5.0.6--5.0.7.sqlsql/spock--5.0.7--5.0.8.sqlsql/spock--5.0.8--6.0.0.sqlsql/spock--6.0.0.sqltests/regress/sql/init_fail.sqltests/tap/scheduletests/tap/t/018_upgrade_schema_match.pl
✅ Files skipped from review due to trivial changes (4)
- sql/spock--5.0.7--5.0.8.sql
- tests/tap/schedule
- include/spock.h
- tests/regress/sql/init_fail.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- sql/spock--5.0.6--5.0.7.sql
| my $Q_TABLES = <<'SQL'; | ||
| SELECT string_agg(table_name, E'\n' ORDER BY table_name) | ||
| FROM information_schema.tables | ||
| WHERE table_schema = 'spock' AND table_type = 'BASE TABLE' | ||
| SQL | ||
|
|
||
| # udt_name catches text vs text[] mismatches (_text = text[]) | ||
| my $Q_COLUMNS = <<'SQL'; | ||
| SELECT string_agg( | ||
| table_name || '.' || column_name || ' ' || udt_name | ||
| || CASE WHEN is_nullable = 'NO' THEN ' NOT NULL' ELSE '' END, | ||
| E'\n' ORDER BY table_name, ordinal_position) | ||
| FROM information_schema.columns | ||
| WHERE table_schema = 'spock' | ||
| SQL | ||
|
|
||
| my $Q_FUNCTIONS = <<'SQL'; | ||
| SELECT string_agg( | ||
| proname | ||
| || '(' || pg_catalog.pg_get_function_arguments(oid) || ')' | ||
| || ':' || prokind::text, | ||
| E'\n' ORDER BY proname, pg_catalog.pg_get_function_arguments(oid)) | ||
| FROM pg_catalog.pg_proc | ||
| WHERE pronamespace = 'spock'::regnamespace | ||
| SQL | ||
|
|
||
| my $Q_VIEWS = <<'SQL'; | ||
| SELECT string_agg(viewname, E'\n' ORDER BY viewname) | ||
| FROM pg_catalog.pg_views | ||
| WHERE schemaname = 'spock' | ||
| SQL | ||
|
|
||
| my $Q_SEQUENCES = <<'SQL'; | ||
| SELECT string_agg(sequence_name, E'\n' ORDER BY sequence_name) | ||
| FROM information_schema.sequences | ||
| WHERE sequence_schema = 'spock' | ||
| SQL | ||
|
|
||
| # View definitions (catches changes to view SQL) | ||
| my $Q_VIEW_DEFS = <<'SQL'; | ||
| SELECT string_agg(viewname || ':' || regexp_replace(definition, '\s+', ' ', 'g'), | ||
| E'\n' ORDER BY viewname) | ||
| FROM pg_catalog.pg_views | ||
| WHERE schemaname = 'spock' | ||
| SQL |
There was a problem hiding this comment.
The catalog diff still misses index definitions and sequence properties.
Right now Q_SEQUENCES only compares names, and there is no index comparison at all. This test can still pass if the upgraded schema is missing spock.resolutions(log_time) or if spock.sub_id_generator differs in AS integer / CYCLE / start settings.
💡 Suggested expansion
+my $Q_INDEXES = <<'SQL';
+SELECT string_agg(
+ indexname || ':' || regexp_replace(indexdef, '\s+', ' ', 'g'),
+ E'\n' ORDER BY indexname)
+FROM pg_catalog.pg_indexes
+WHERE schemaname = 'spock'
+SQL
+
my $Q_SEQUENCES = <<'SQL';
SELECT string_agg(sequence_name, E'\n' ORDER BY sequence_name)
FROM information_schema.sequences
WHERE sequence_schema = 'spock'
SQL
+
+my $Q_SEQUENCE_DEFS = <<'SQL';
+SELECT string_agg(
+ sequencename || ':' || data_type || ':' || start_value || ':' ||
+ minimum_value || ':' || maximum_value || ':' || increment_by || ':' || cycle,
+ E'\n' ORDER BY sequencename)
+FROM pg_catalog.pg_sequences
+WHERE schemaname = 'spock'
+SQL
@@
compare_category('Sequences',
psql_query($PORT_UPG, $Q_SEQUENCES),
psql_query($PORT_NEW, $Q_SEQUENCES));
+
+compare_category('Sequence definitions',
+ psql_query($PORT_UPG, $Q_SEQUENCE_DEFS),
+ psql_query($PORT_NEW, $Q_SEQUENCE_DEFS));
+
+compare_category('Indexes',
+ psql_query($PORT_UPG, $Q_INDEXES),
+ psql_query($PORT_NEW, $Q_INDEXES));Also applies to: 445-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 286 - 330, The test's
catalog queries miss index definitions and sequence properties; update the diff
queries by adding a new Q_INDEXES (e.g., query pg_catalog.pg_indexes or join
pg_class/pg_index/pg_attribute) that returns a deterministic string per index
including schemaname.table, indexname, indexdef (or columns/order, uniqueness,
predicate, expression) ordered by index name, and extend Q_SEQUENCES to include
sequence attributes (data_type/udt_name, start_value, minimum_value,
maximum_value, increment_by, is_cycled, last_value/owned_by if relevant) so the
test will detect changes like spock.resolutions(log_time) index removal or
differences in spock.sub_id_generator AS integer/CYCLE/start; apply the same
sequence expansion where the other query copy exists (lines noted in the
comment).
Bump version to 6.0.0 and add upgrade path from 5.0.7
SPOCK_VERSIONto"6.0.0"andSPOCK_VERSION_NUMto60000ininclude/spock.hspock--6.0.0-devel.sql→spock--6.0.0.sqlandspock--5.0.7--6.0.0-devel.sql→spock--5.0.8--6.0.0.sqlspock--5.0.6--5.0.7.sqlstep upgrade script (pause/resume workers,wait_for_sync_eventrewrite,sync_eventoptionaltransactionalarg,sub_skip_schemacolumn relabel)progress/lag_trackerviews, addssub_id_generatorsequence, migrates conflict type names, and addscleanup_resolutions+ subscription stats functions018_upgrade_schema_matchto verify that a 5.0.8 → 6.0.0 upgrade produces a schema identical to a fresh 6.0.0 install